fix($parse): standardize one-time literal vs non-literal and interceptors #15858

jbedard · 2017-03-24T08:52:46Z

Previously literal one-time bindings did not use the expression inputs, causing infinite digest issues with literal values. This often forces the use of deepEquals when watching one-time literals.

ng-class is one example of deepEquals which is no longer required (and had to be updated to keep all tests passing).

This one-time/literal behavior is now also consistently propagated through interceptors (and had to be updated to keep all tests passing).

The ngClass/interceptor part was not originally planned but had to be changed to keep tests passing. Maybe this can be split into smaller commits? But it's mainly just deleting...!

gkalpak · 2017-03-24T10:16:11Z

src/ng/directive/ngClass.js

@@ -97,15 +90,9 @@ function classDirective(name, selector) {
          oldModulo = newModulo;
        }

-        function ngClassOneTimeWatchAction(newClassValue) {
+        function ngClassWatchAction(newClassValue) {
          var newClassString = toClassString(newClassValue);


Since toClassString is being used as an interceptor, is it necessary to pass it through toClassString again here?

I think you're correct, updated...

Unfortunately we were wrong. In the case of one-time bindings the pre-interceptor value is returned up until that value passes the one-time test:

angular.js/src/ng/parse.js

Line 1933 in 2b0c050

return isDefined(value) ? result : value;

I've updated it to do this toClassString conditionally and added a comment.

gkalpak · 2017-03-27T09:19:05Z

src/ng/parse.js

-      }
+    function isAllDefined(value) {
+      var allDefined = true;
+      forEach(value, function(val) {


Nit: Not directly related to this PR, but I think it makes sense to exit early once an undefined value is found.

I think value can be an object or array which forEach currently handles or us. Did you have something in mind?

Since this is currently only used for object/array literal cases, we could split this into separate object/array helper methods (one using for-in one using array.every?)...

I agree it is not worth it. It is just for one-time literals and it will only matter if one has huge literals which don't settle quickly (a very unlikely usecase).

…tors Previously literal one-time bindings did not use the expression `inputs`, causing infinite digest issues with literal values. This often forces the use of deepEquals when watching one-time literals. `ng-class` is one example of deepEquals which is no longer required. This one-time/literal behavior is now also consistently propogated through interceptors.

gkalpak · 2017-03-29T09:10:47Z

src/ng/directive/ngClass.js

-          if (newClassString !== oldClassString) {
-            ngClassWatchAction(newClassString);
+        function ngClassWatchAction(newClassString) {
+          //When using a one-time binding the newClassString will return


You realy don't like spaces after //, do you 😛

Bad (or just different!) habit I guess :)

gkalpak · 2017-03-29T09:17:05Z

src/ng/directive/ngClass.js

+        function ngClassWatchAction(newClassString) {
+          //When using a one-time binding the newClassString will return
+          //the pre-interceptor value until the one-time is complete
+          if (newClassString && typeof newClassString !== "string") {


Why not just !isString(newClassString)?
Also, I don't think it makes much difference over always applying toClassString() (which just does isArray() and isObject()).

I've updated to isString. I kind of like an if block just so it's obvious that the comment applies to the full block, nothing more nothing less...

Narretz · 2017-03-27T07:41:52Z

src/ng/parse.js

@@ -1859,6 +1859,7 @@ function $ParseProvider() {
    }

    function oneTimeWatchDelegate(scope, listener, objectEquality, parsedExpression, prettyPrintExpression) {
+      var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined;


This could have a better name, e.g. isDone, fulfillsDoneCriteria, because it's not very descriptive when called later on

Good idea, done.

…nterceptors

jbedard · 2017-03-31T04:47:47Z

I think this is ready, PTAL.

googlebot · 2017-03-31T07:13:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-03-31T07:13:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

…tors Previously literal one-time bindings did not use the expression `inputs`, causing infinite digest issues with literal values. This often forces the use of deepEquals when watching one-time literals. `ng-class` is one example of deepEquals which is no longer required. This one-time/literal behavior is now also consistently propogated through interceptors. Closes #15858

googlebot added the cla: yes label Mar 24, 2017

jbedard force-pushed the one-time-literal branch from fbb62fa to a0bb123 Compare March 24, 2017 16:14

gkalpak reviewed Mar 27, 2017

View reviewed changes

jbedard force-pushed the one-time-literal branch 2 times, most recently from 775281c to 1ef435b Compare March 29, 2017 07:29

jbedard force-pushed the one-time-literal branch from 1ef435b to a58a052 Compare March 29, 2017 07:32

gkalpak reviewed Mar 29, 2017

View reviewed changes

Narretz suggested changes Mar 29, 2017

View reviewed changes

fixup! fix($parse): standardize one-time literal vs non-literal and i…

2087b25

…nterceptors

gkalpak approved these changes Mar 31, 2017

View reviewed changes

update variable name

2b6c11b

googlebot added cla: no and removed cla: yes labels Mar 31, 2017

Narretz approved these changes Mar 31, 2017

View reviewed changes

Narretz merged commit 189461f into angular:master Mar 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($parse): standardize one-time literal vs non-literal and interceptors #15858

fix($parse): standardize one-time literal vs non-literal and interceptors #15858

jbedard commented Mar 24, 2017

gkalpak Mar 24, 2017

jbedard Mar 28, 2017

jbedard Mar 29, 2017

gkalpak Mar 27, 2017

jbedard Mar 28, 2017

gkalpak Mar 28, 2017

gkalpak Mar 29, 2017

jbedard Mar 30, 2017

gkalpak Mar 29, 2017

jbedard Mar 30, 2017

Narretz Mar 27, 2017

jbedard Mar 30, 2017

jbedard commented Mar 31, 2017

googlebot commented Mar 31, 2017

googlebot commented Mar 31, 2017

fix($parse): standardize one-time literal vs non-literal and interceptors #15858

fix($parse): standardize one-time literal vs non-literal and interceptors #15858

Conversation

jbedard commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Mar 31, 2017

googlebot commented Mar 31, 2017

googlebot commented Mar 31, 2017